Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: Detect board port change after upload #2253

Merged
merged 18 commits into from
Aug 18, 2023

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Aug 1, 2023

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

The CLI is now able to detect the port where a board re-attaches after an upload (this is particularly useful for boards with native USB connections that require disconnection of the communication port to perform the sketch upload).
Previously this task was performed by the Arduino IDE with mixed results, for more info see #2245.

What is the current behavior?

After an upload if the board change port this is not detected.

What is the new behavior?

:~/Arduino/Blink$ arduino-cli upload -b arduino:samd:nano_33_iot -p /dev/ttyACM1
Atmel SMART device 0x10010005 found
Device       : ATSAMD21G18A
Chip ID      : 10010005
Version      : v2.0 [Arduino:XYZ] Apr 19 2019 14:38:48
Address      : 8192
Pages        : 3968
Page Size    : 64 bytes
Total Size   : 248KB
Planes       : 1
Lock Regions : 16
Locked       : none
Security     : false
Boot Flash   : true
BOD          : true
BOR          : true
Arduino      : FAST_CHIP_ERASE
Arduino      : FAST_MULTI_PAGE_WRITE
Arduino      : CAN_CHECKSUM_MEMORY_BUFFER
Erase flash
done in 0.818 seconds

Write 11404 bytes to flash (179 pages)
[==============================] 100% (179/179 pages)
done in 0.071 seconds

Verify 11404 bytes of flash with checksum.
Verify successful
done in 0.009 seconds
CPU reset.
New upload port: /dev/ttyACM0 (serial)            <------

The same information is available through JSON output too:

:~/Arduino/Blink$ arduino-cli upload -b arduino:samd:nano_33_iot -p /dev/ttyACM1 --format json
{
  "stdout": "Atmel SMART device 0x10010005 found\nDevice       : ATSAMD21G18A\nChip ID      : 10010005\nVersion      : v2.0 [Arduino:XYZ] Apr 19 2019 14:38:48\nAddress      : 8192\nPages        : 3968\nPage Size    : 64 bytes\nTotal Size   : 248KB\nPlanes       : 1\nLock Regions : 16\nLocked       : none\nSecurity     : false\nBoot Flash   : true\nBOD          : true\nBOR          : true\nArduino      : FAST_CHIP_ERASE\nArduino      : FAST_MULTI_PAGE_WRITE\nArduino      : CAN_CHECKSUM_MEMORY_BUFFER\nErase flash\ndone in 0.819 seconds\n\nWrite 11404 bytes to flash (179 pages)\n\r[==========                    ] 35% (64/179 pages)\r[=====================         ] 71% (128/179 pages)\r[==============================] 100% (179/179 pages)\ndone in 0.070 seconds\n\nVerify 11404 bytes of flash with checksum.\nVerify successful\ndone in 0.009 seconds\nCPU reset.\n",
  "stderr": "",
  "updated_upload_port": {
    "address": "/dev/ttyACM0",                 <-------
    "label": "/dev/ttyACM0",
    "protocol": "serial",
    "protocol_label": "Serial Port (USB)",
    "properties": {
      "pid": "0x8057",
      "serialNumber": "2B59DD8F50534B54332E3120FF03113B",
      "vid": "0x2341"
    },
    "hardware_id": "2B59DD8F50534B54332E3120FF03113B"
  }
}

Does this PR introduce a breaking change, and is titled accordingly?

Yes

Other information

Fix #2245

@cmaglie cmaglie self-assigned this Aug 1, 2023
@cmaglie cmaglie added priority: high Resolution is a high priority topic: code Related to content of the project itself topic: gRPC Related to the gRPC interface criticality: highest Of highest impact type: enhancement Proposed improvement labels Aug 1, 2023
kittaakos pushed a commit to kittaakos/arduino-ide that referenced this pull request Aug 2, 2023
 - update firmware uploader to `2.3.0`,
 - new gRPC API

Signed-off-by: Akos Kitta <[email protected]>
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Patch coverage: 58.26% and no project coverage change.

Comparison is base (246adf9) 62.94% compared to head (862bbe2) 62.94%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #2253    +/-   ##
========================================
  Coverage   62.94%   62.94%            
========================================
  Files         220      221     +1     
  Lines       19540    19724   +184     
========================================
+ Hits        12299    12415   +116     
- Misses       6151     6207    +56     
- Partials     1090     1102    +12     
Flag Coverage Δ
unit 62.94% <58.26%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
commands/daemon/daemon.go 27.67% <0.00%> (-1.03%) ⬇️
commands/upload/burnbootloader.go 0.00% <0.00%> (ø)
commands/board/list.go 58.70% <42.85%> (-1.50%) ⬇️
internal/cli/upload/upload.go 60.68% <53.33%> (+0.10%) ⬆️
commands/upload/upload.go 73.86% <63.19%> (-0.83%) ⬇️
internal/cli/compile/compile.go 73.43% <66.66%> (-0.54%) ⬇️
arduino/discovery/discovery.go 44.55% <69.23%> (+2.31%) ⬆️
internal/algorithms/channels.go 76.92% <76.92%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arduino CLI should always return the port after an upload, even in the case where no port change is expected. The consumer shouldn't be required to implement "if not updated_upload_port, use original port" logic.

The whole point is that all the logic for determining which port should be selected after an upload should be implemented in Arduino CLI. The consumer should be able to simply select the port Arduino CLI tells it to select in all cases.

Arduino CLI should always return the port after an upload, even in the case
where no port change is expected. The consumer shouldn't be required to
implement "if not updated_upload_port, use original port" logic.

The whole point is that all the logic for determining which port should be
selected after an upload should be implemented in Arduino CLI. The consumer
should be able to simply select the port Arduino CLI tells it to select in
all cases.
@cmaglie cmaglie marked this pull request as ready for review August 9, 2023 12:59
@cmaglie cmaglie added this to the Arduino CLI 1.0 milestone Aug 9, 2023
@cmaglie
Copy link
Member Author

cmaglie commented Aug 9, 2023

Arduino CLI should always return the port after an upload, even in the case where no port change is expected

I've implemented the behavior above.

@kittaakos
Copy link
Contributor

I received specific steps to force the port change with Uno R4 WiFi. In this case, the updated_upload_port does not tell the new port, but the port has changed.

I have a procedure you can use to get an address change on macOS (as well as Linux and Windows) using an UNO R4 WiFi board:

  1. Press the reset button on the board twice quickly. You should now see the "L" LED pulsing. The address will be something like /dev/cu.usbmodemDC5475C4C6542
  2. Upload the following sketch:
#include <HID.h>
void setup() {}
void loop() {}

After the upload, the address will be something like /dev/cu.usbmodem2214101

Note that after you upload that sketch, the next attempt to upload a sketch to your board will fail (arduino/ArduinoCore-renesas#73). The workaround is to press the reset button twice quickly before uploading. The problem (and the address change) will no longer occur after you have successfully uploaded any other sketch that doesn't use HID to the board so performing the instructions I provided above doesn't do any permanent harm to the board.


Steps:

./arduino-cli version                 
arduino-cli  Version: git-snapshot Commit: 990d93a5 Date: 2023-08-10T14:51:50Z
./arduino-cli board list --format json
[
  {
    "port": {
      "address": "/dev/cu.BLTH",
      "label": "/dev/cu.BLTH",
      "protocol": "serial",
      "protocol_label": "Serial Port"
    }
  },
  {
    "port": {
      "address": "/dev/cu.Bluetooth-Incoming-Port",
      "label": "/dev/cu.Bluetooth-Incoming-Port",
      "protocol": "serial",
      "protocol_label": "Serial Port"
    }
  },
  {
    "matching_boards": [
      {
        "name": "Arduino UNO R4 WiFi",
        "fqbn": "arduino:renesas_uno:unor4wifi"
      }
    ],
    "port": {
      "address": "/dev/cu.usbmodemDC5475C56BE02",
      "label": "/dev/cu.usbmodemDC5475C56BE02",
      "protocol": "serial",
      "protocol_label": "Serial Port (USB)",
      "properties": {
        "pid": "0x1002",
        "serialNumber": "DC5475C56BE0",
        "vid": "0x2341"
      },
      "hardware_id": "DC5475C56BE0"
    }
  }
]
cat ~/Documents/Arduino/r4/r4.ino     
#include <HID.h>
void setup() {}
void loop() {}% 
 ./arduino-cli upload -b arduino:renesas_uno:unor4wifi -p /dev/cu.usbmodemDC5475C56BE02 ~/Documents/Arduino/r4 --format json
{
  "stdout": "Erase flash\n\nDone in 0.001 seconds\nWrite 46784 bytes to flash (12 pages)\n\r[                              ] 0% (0/12 pages)\r[==                            ] 8% (1/12 pages)\r[=====                         ] 16% (2/12 pages)\r[=======                       ] 25% (3/12 pages)\r[==========                    ] 33% (4/12 pages)\r[============                  ] 41% (5/12 pages)\r[===============               ] 50% (6/12 pages)\r[=================             ] 58% (7/12 pages)\r[====================          ] 66% (8/12 pages)\r[======================        ] 75% (9/12 pages)\r[=========================     ] 83% (10/12 pages)\r[===========================   ] 91% (11/12 pages)\r[==============================] 100% (12/12 pages)\nDone in 2.968 seconds\n",
  "stderr": "",
  "updated_upload_port": {
    "address": "/dev/cu.usbmodemDC5475C56BE02",
    "label": "/dev/cu.usbmodemDC5475C56BE02",
    "protocol": "serial",
    "protocol_label": "Serial Port (USB)",
    "properties": {
      "pid": "0x1002",
      "serialNumber": "DC5475C56BE0",
      "vid": "0x2341"
    },
    "hardware_id": "DC5475C56BE0"
  }
}

The updated_upload_port#address is still /dev/cu.usbmodemDC5475C56BE02 👆, but the board list shows different: /dev/cu.usbmodem14201 👇.

./arduino-cli board list --format json
[
  {
    "port": {
      "address": "/dev/cu.BLTH",
      "label": "/dev/cu.BLTH",
      "protocol": "serial",
      "protocol_label": "Serial Port"
    }
  },
  {
    "port": {
      "address": "/dev/cu.Bluetooth-Incoming-Port",
      "label": "/dev/cu.Bluetooth-Incoming-Port",
      "protocol": "serial",
      "protocol_label": "Serial Port"
    }
  },
  {
    "matching_boards": [
      {
        "name": "Arduino UNO R4 WiFi",
        "fqbn": "arduino:renesas_uno:unor4wifi"
      }
    ],
    "port": {
      "address": "/dev/cu.usbmodem14201",
      "label": "/dev/cu.usbmodem14201",
      "protocol": "serial",
      "protocol_label": "Serial Port (USB)",
      "properties": {
        "pid": "0x006D",
        "serialNumber": "3507110A35323634DB5233324B572D0A",
        "vid": "0x2341"
      },
      "hardware_id": "3507110A35323634DB5233324B572D0A"
    }
  }
]

CLI version: 990d93a

I don't know whether this PR should cover this use case. Can you please help, @per1234? Thank you!

@kittaakos
Copy link
Contributor

kittaakos commented Aug 10, 2023

I am using d1869b6, and after manually running ~20 consecutive uploads, I no longer see the bug. ✅

arduino-cli  Version: git-snapshot Commit: d1869b66 Date: 2023-08-11T11:20:14Z

This one is more likely a bug. I do not have the steps to reproduce it consistently, but I have noticed some odd behavior in IDE2, and it happens with pure CLI after the upload (repeat the upload a few times (~10), and it will break once).

I have a MKR 1000 attached on /dev/cu.usbmodem14101. I do an upload and the updated_upload_port#address is strange:

./arduino-cli upload -b arduino:samd:mkr1000 -p /dev/cu.usbmodem14101 ~/Documents/Arduino/monitor3 --format json
{
  "stdout": "Atmel SMART device 0x10010005 found\nDevice       : ATSAMD21G18A\nChip ID      : 10010005\nVersion      : v2.0 [Arduino:XYZ] Dec 20 2016 15:36:43\nAddress      : 8192\nPages        : 3968\nPage Size    : 64 bytes\nTotal Size   : 248KB\nPlanes       : 1\nLock Regions : 16\nLocked       : none\nSecurity     : false\nBoot Flash   : true\nBOD          : true\nBOR          : true\nArduino      : FAST_CHIP_ERASE\nArduino      : FAST_MULTI_PAGE_WRITE\nArduino      : CAN_CHECKSUM_MEMORY_BUFFER\nErase flash\ndone in 0.700 seconds\n\nWrite 11344 bytes to flash (178 pages)\n\r[==========                    ] 35% (64/178 pages)\r[=====================         ] 71% (128/178 pages)\r[==============================] 100% (178/178 pages)\ndone in 0.074 seconds\n\nVerify 11344 bytes of flash with checksum.\nVerify successful\ndone in 0.010 seconds\nCPU reset.\n",
  "stderr": "",
  "updated_upload_port": {
    "address": "/dev/tty.usbmodem14101",
    "label": "/dev/cu.usbmodem14101",
    "protocol": "serial",
    "protocol_label": "Serial Port (USB)",
    "properties": {
      "pid": "0x804E",
      "serialNumber": "94A3397C5150435437202020FF150838",
      "vid": "0x2341"
    },
    "hardware_id": "94A3397C5150435437202020FF150838"
  }
}

updated_upload_port#address is /dev/tty.usbmodem14101, but the label is /dev/cu.usbmodem14101. The board list does not see /dev/tty.

./arduino-cli board list --format json
[
  {
    "port": {
      "address": "/dev/cu.BLTH",
      "label": "/dev/cu.BLTH",
      "protocol": "serial",
      "protocol_label": "Serial Port"
    }
  },
  {
    "port": {
      "address": "/dev/cu.Bluetooth-Incoming-Port",
      "label": "/dev/cu.Bluetooth-Incoming-Port",
      "protocol": "serial",
      "protocol_label": "Serial Port"
    }
  },
  {
    "matching_boards": [
      {
        "name": "Arduino MKR1000",
        "fqbn": "arduino:samd:mkr1000"
      }
    ],
    "port": {
      "address": "/dev/cu.usbmodem14101",
      "label": "/dev/cu.usbmodem14101",
      "protocol": "serial",
      "protocol_label": "Serial Port (USB)",
      "properties": {
        "pid": "0x804E",
        "serialNumber": "94A3397C5150435437202020FF150838",
        "vid": "0x2341"
      },
      "hardware_id": "94A3397C5150435437202020FF150838"
    }
  }
]

It's currently a blocking issue in IDE2. Thanks!

@per1234
Copy link
Contributor

per1234 commented Aug 11, 2023

I received specific steps to force the port change with Uno R4 WiFi. In this case, the updated_upload_port does not tell the new port, but the port has changed.

Hi @kittaakos. I apologize for the confusion. I intended to circle back to our Slack convo about this after I had investigated but hadn't found the time.

I provided these instructions purely as a response to the request for demonstrations of an upload causing a port change. I hadn't looked into how the system implemented in this PR would work at that time so I didn't realize that there were some unique considerations for this particular demonstration.

A port change takes some time for the operating system to de-enumerate the old port and then enumerate the new port. So Arduino CLI's system for detecting a port change requires it to wait some time while watching for the appearance of a new port, timing out and falling back to using the original port if the new port doesn't appear within the timeout duration.

Some boards (e.g., classic UNO, Mega, Nano Every) will never experience a port change on upload. Since it would be inefficient for Arduino CLI to wait around for a port change in this case it is possible for the platform author to configure the board definition to skip the wait step by setting the upload.wait_for_upload_port property to false in the board definition.

So for boards with this upload.wait_for_upload_port property set to false, Arduino CLI simply returns the original port in the updated_upload_port of the upload response.

if uploadProperties.GetBoolean("upload.wait_for_upload_port") && watch != nil {
go detectUploadPort(uploadCtx, port, watch, updatedUploadPort)

There is an unusual situation with the UNO R4 WiFi where, where the average sketch will not produce a port change due to the board having a dedicated USB "bridge" module, but if the sketch uses USB HID capabilities (which, in a innovative approach, are provided by the "bridge" chip) then the port change does occur.

The board definition of the UNO R4 WiFi sets the upload.wait_for_upload_port property to false:

https://github.com/arduino/ArduinoCore-renesas/blob/1.0.2/boards.txt#L138

This is arguably incorrect, but setting it to true would cause the wait timeout delay to be added to the upload time whenever the user is not using a sketch with HID (which is probably 95% of the time). So the decision was made to accept a poor user experience when the HID capabilities are used:

arduino/ArduinoCore-renesas#74 (comment)

The mitigation measure for the existing poor user experience has been to document the special procedures that are required when using the board's HID capabilities (arduino/docs-content#1246, arduino/help-center-content#258). I'll have to add the requirement of manually selecting the post-upload port to that documentation once we ship arduino/arduino-ide#2165

It is possible to adjust the configuration of the UNO R4 WiFi board definition to simulate a port change on a board that is correctly configured for such a thing:

  1. Open the file at this path:
    ~/Library/Arduino15/packages/arduino/hardware/renesas_uno/1.0.2/boards.txt
    
  2. Change line 138 from:
    unor4wifi.upload.wait_for_upload_port=false
    
    to:
    unor4wifi.upload.wait_for_upload_port=true
    
  3. Save the file.
  4. Restart Arduino IDE if it is running to trigger a reload of the updated platform configuration file.

Unfortunately I find that Arduino CLI is still unable to correctly identify the post-upload port. I'm not sure why. I see the port is seen, but has a different hardware_id field value:

time="2023-08-10T21:02:19-07:00" level=trace msg="New upload port candidate" event="event_type:\"add\"  port:{matching_boards:{name:\"Arduino UNO R4 WiFi\"  fqbn:\"arduino:renesas_uno:unor4wifi\"}  port:{address:\"COM12\"  label:\"COM12\"  protocol:\"serial\"  protocol_label:\"Serial Port (USB)\"  properties:{key:\"pid\"  value:\"0x006D\"}  properties:{key:\"serialNumber\"  value:\"331226165931383508BC33324B572E41\"}  properties:{key:\"vid\"  value:\"0x2341\"}  hardware_id:\"331226165931383508BC33324B572E41\"}}" task=port_detection
time="2023-08-10T21:02:19-07:00" level=trace msg="New candidate port did not match desired HW ID, keep watching..." task=port_detection
time="2023-08-10T21:02:23-07:00" level=trace msg="Timeout waiting for candidate port" task=port_detection

@per1234
Copy link
Contributor

per1234 commented Aug 11, 2023

updated_upload_port#address is /dev/tty.usbmodem14101, but the label is /dev/cu.usbmodem14101

I am also able to reproduce this. I see that the updated_upload_port.address field value is that of the port used during the upload instead of the post-upload port:

$ arduino-cli upload -b arduino:samd:mkrzero -p COM32 --format json --log-file logs.log  --log-level trace
{
  "stdout": "Atmel SMART device 0x10010005 found\r\nDevice       : ATSAMD21G18A\r\nChip ID      : 10010005\r\nVersion      : v2.0 [Arduino:XYZ] Apr 11 2019 13:09:53\r\nAddress      : 8192\r\nPages        : 3968\r\nPage Size    : 64 bytes\r\nTotal Size   : 248KB\r\nPlanes       : 1\r\nLock Regions : 16\r\nLocked       : none\r\nSecurity     : false\r\nBoot Flash   : true\r\nBOD          : false\r\nBOR          : false\r\nArduino      : FAST_CHIP_ERASE\r\nArduino      : FAST_MULTI_PAGE_WRITE\r\nArduino      : CAN_CHECKSUM_MEMORY_BUFFER\r\nErase flash\r\ndone in 0.915 seconds\r\n\r\nWrite 11068 bytes to flash (173 pages)\r\n\r[===========                   ] 36% (64/173 pages)\r[======================        ] 73% (128/173 pages)\r[==============================] 100% (173/173 pages)\r\ndone in 0.083 seconds\r\n\r\nVerify 11068 bytes of flash with checksum.\r\nVerify successful\r\ndone in 0.010 seconds\r\nCPU reset.\r\n",
  "stderr": "",
  "updated_upload_port": {
    "address": "COM33",
    "label": "COM32",
    "protocol": "serial",
    "protocol_label": "Serial Port (USB)",
    "properties": {
      "pid": "0x804F",
      "serialNumber": "F0F2036051504C3750202020FF0F2520",
      "vid": "0x2341"
    },
    "hardware_id": "F0F2036051504C3750202020FF0F2520"
  }
}

$ arduino-cli board list --format json
[
  {
    "matching_boards": [
      {
        "name": "Arduino MKRZERO",
        "fqbn": "arduino:samd:mkrzero"
      }
    ],
    "port": {
      "address": "COM32",
      "label": "COM32",
      "protocol": "serial",
      "protocol_label": "Serial Port (USB)",
      "properties": {
        "pid": "0x804F",
        "serialNumber": "F0F2036051504C3750202020FF0F2520",
        "vid": "0x2341"
      },
      "hardware_id": "F0F2036051504C3750202020FF0F2520"
    }
  },
time="2023-08-10T21:46:08-07:00" level=trace msg="New upload port candidate" event="event_type:\"add\"  port:{matching_boards:{name:\"Arduino MKRZERO\"  fqbn:\"arduino:samd:mkrzero\"}  port:{address:\"COM32\"  label:\"COM32\"  protocol:\"serial\"  protocol_label:\"Serial Port (USB)\"  properties:{key:\"pid\"  value:\"0x804F\"}  properties:{key:\"serialNumber\"  value:\"F0F2036051504C3750202020FF0F2520\"}  properties:{key:\"vid\"  value:\"0x2341\"}  hardware_id:\"F0F2036051504C3750202020FF0F2520\"}}" task=port_detection
time="2023-08-10T21:46:08-07:00" level=trace msg="Found new upload port!" task=port_detection

Previously only the pointer was copied, thus making changes in
`actualPort` to be reflected also to `port`. This lead to some weird
result in the `updatedUploadPort` result:

{
  "stdout": "Verify 11344 bytes of flash with checksum.\nVerify successful\ndone in 0.010 seconds\nCPU reset.\n",
  "stderr": "",
  "updated_upload_port": {
    "address": "/dev/tty.usbmodem14101",     <------- this address...
    "label": "/dev/cu.usbmodem14101",        <------- ...is different from the label
    "protocol": "serial",
    "protocol_label": "Serial Port (USB)",
    "properties": {
      "pid": "0x804E",
      "serialNumber": "94A3397C5150435437202020FF150838",
      "vid": "0x2341"
    },
    "hardware_id": "94A3397C5150435437202020FF150838"
  }
}
@cmaglie
Copy link
Member Author

cmaglie commented Aug 11, 2023

updated_upload_port#address is /dev/tty.usbmodem14101, but the label is /dev/cu.usbmodem14101. The board list does not see /dev/tty.

I've pushed two commits that should fix this problem.

We must acesss the gRPC API only until we cross the `command` package
border. Once we are inside the `command` package we should use the
internal API only.
Now the upload detects cases when the upload port is "unstable", i.e.
the port changes even if it shouldn't (because the wait_for_upload_port
property in boards.txt is set to false).

This change should make the upload process more resilient.
@cmaglie
Copy link
Member Author

cmaglie commented Aug 14, 2023

I've implemented a workaround for the weird port change on the Uno R4, so the detection now work more or less like this:

  • if wait_for_upload_port is true: after the upload wait for a new port to become available:

    • if the port has the same hardware ID as the original upload port, then use it immediately
    • otherwise keep the port but wait up to 5 seconds in case another port that has a matching hardware ID shows up
  • if wait_for_upload_port is false: after the upload wait 1 second and check if the upload port is still alive:

    • if it's still alive, use the old upload port
    • otherwise consider the port lost, and wait for an upload port using the same procedure as for wait_for_upload_port == true

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UPDATE: Fixed by e12a068

Panic when upload causes disappearance of port

On boards with native USB, the USB stack that produces the USB CDC serial port used for uploading is running on the same microcontroller as the sketch program. This means that, when working with these board, it is common for sketch uploads to cause the disappearance of the port, either unexpectedly due to a bug such as a divide by zero or expectedly such as when putting the MCU to sleep.

🐛 If a sketch upload causes the disappearance of the port, Arduino CLI panics.

To reproduce

Equipment

Steps

Upload a sketch that causes the loss of the port of your board:

$ ./arduino-cli version

arduino-cli.exe  Version: git-snapshot Commit: e9e5fbdd Date: 2023-08-16T03:41:25Z

$ mkdir "/tmp/NoInterruptsSketch"

$ printf "void setup() {noInterrupts();}\nvoid loop() {}\n" > "/tmp/NoInterruptsSketch/NoInterruptsSketch.ino"

$ ./arduino-cli compile --fqbn arduino:avr:leonardo

[...]

$ ./arduino-cli upload --fqbn arduino:avr:leonardo --port COM23 --verbose "/tmp/NoInterruptsSketch"

Performing 1200-bps touch reset on serial port COM23
Waiting for upload port...
Upload port found on COM24
"C:\Users\per\AppData\Local\Arduino15\packages\arduino\tools\avrdude\6.3.0-arduino17/bin/avrdude" "-CC:\Users\per\AppData\Local\Arduino15\packages\arduino\tools\avrdude\6.3.0-arduino17/etc/avrdude.conf" -v -V -patmega32u4 -cavr109 "-PCOM24" -b57600 -D "-Uflash:w:C:\Users\per\AppData\Local\Temp\arduino\sketches\892D5E2A267501D13FBB3246DF62F6BE/NoInterruptsSketch.ino.hex:i"

avrdude: Version 6.3-20190619
         Copyright (c) 2000-2005 Brian Dean, http://www.bdmicro.com/
         Copyright (c) 2007-2014 Joerg Wunsch

         System wide configuration file is "C:\Users\per\AppData\Local\Arduino15\packages\arduino\tools\avrdude\6.3.0-arduino17/etc/avrdude.conf"

         Using Port                    : COM24
         Using Programmer              : avr109
         Overriding Baud Rate          : 57600
         AVR Part                      : ATmega32U4
         Chip Erase delay              : 9000 us
         PAGEL                         : PD7
         BS2                           : PA0
         RESET disposition             : dedicated
         RETRY pulse                   : SCK
         serial program mode           : yes
         parallel program mode         : yes
         Timeout                       : 200
         StabDelay                     : 100
         CmdexeDelay                   : 25
         SyncLoops                     : 32
         ByteDelay                     : 0
         PollIndex                     : 3
         PollValue                     : 0x53
         Memory Detail                 :

                                  Block Poll               Page                       Polled
           Memory Type Mode Delay Size  Indx Paged  Size   Size #Pages MinW  MaxW   ReadBack
           ----------- ---- ----- ----- ---- ------ ------ ---- ------ ----- ----- ---------
           eeprom        65    20     4    0 no       1024    4      0  9000  9000 0x00 0x00
           flash         65     6   128    0 yes     32768  128    256  4500  4500 0x00 0x00
           lfuse          0     0     0    0 no          1    0      0  9000  9000 0x00 0x00
           hfuse          0     0     0    0 no          1    0      0  9000  9000 0x00 0x00
           efuse          0     0     0    0 no          1    0      0  9000  9000 0x00 0x00
           lock           0     0     0    0 no          1    0      0  9000  9000 0x00 0x00
           calibration    0     0     0    0 no          1    0      0     0     0 0x00 0x00
           signature      0     0     0    0 no          3    0      0     0     0 0x00 0x00

         Programmer Type : butterfly
         Description     : Atmel AppNote AVR109 Boot Loader

Connecting to programmer: .
Found programmer: Id = "CATERIN"; type = S
    Software Version = 1.0; No Hardware Version given.
Programmer supports auto addr increment.
Programmer supports buffered memory access with buffersize=128 bytes.

Programmer supports the following devices:
    Device code: 0x44

avrdude: devcode selected: 0x44
avrdude: AVR device initialized and ready to accept instructions

Reading | ################################################## | 100% 0.00s

avrdude: Device signature = 0x1e9587 (probably m32u4)
avrdude: reading input file "C:\Users\per\AppData\Local\Temp\arduino\sketches\892D5E2A267501D13FBB3246DF62F6BE/NoInterruptsSketch.ino.hex"
avrdude: writing flash (3464 bytes):

Writing | ################################################## | 100% 0.27s

avrdude: 3464 bytes of flash written

avrdude done.  Thank you.

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x50 pc=0xe60dea]

goroutine 1 [running]:
github.com/arduino/arduino-cli/arduino/discovery.(*Port).ToRPC(...)
        E:/git/arduino-cli/arduino/discovery/discovery.go:108
github.com/arduino/arduino-cli/commands/upload.runProgramAction(0xc000c6a700, 0x65d6de?, {0x0, 0x0}, {0x0, 0x0}, {0xc00011a060, 0x14}, 0x1a321a0?, {0x0, ...}, ...)
        E:/git/arduino-cli/commands/upload/upload.go:515 +0x54ca
github.com/arduino/arduino-cli/commands/upload.Upload({0x19d9060?, 0x0?}, 0xc00010c000, {0x13a2a60, 0xc0001151a0}, {0x13a2a60, 0xc0001151b8})
        E:/git/arduino-cli/commands/upload/upload.go:146 +0x468
github.com/arduino/arduino-cli/internal/cli/upload.runUploadCommand(0xc00038c280?, {0xc000170cc0, 0x1, 0x6?})
        E:/git/arduino-cli/internal/cli/upload/upload.go:171 +0xbba
github.com/spf13/cobra.(*Command).execute(0xc00038c280, {0xc000170c60, 0x6, 0x6})
        C:/Users/per/go/pkg/mod/github.com/spf13/[email protected]/command.go:860 +0x663
github.com/spf13/cobra.(*Command).ExecuteC(0xc0002c0500)
        C:/Users/per/go/pkg/mod/github.com/spf13/[email protected]/command.go:974 +0x3bd
github.com/spf13/cobra.(*Command).Execute(0x0?)
        C:/Users/per/go/pkg/mod/github.com/spf13/[email protected]/command.go:902 +0x19
main.main()
        E:/git/arduino-cli/main.go:31 +0xea

🐛 The process panicked.

Expected behavior

Arduino CLI handles the disappearance of the board's port after an upload gracefully.

Arduino CLI version

e9e5fbd

Operating system

Windows 11

Additional context

I bisected the regression to e9e5fbd (does not occur at the previous commit f8394fb).

Recovering the board from the no port state

If you performed the demonstration I provided above, the board will be in a state where it can't be uploaded to as usual. It can be recovered by performing the following procedure:

  1. Prepare a sketch that doesn't contain any problematic code:
    $ arduino-cli sketch new "/tmp/BareMinimum"
    
  2. Invoke an arduino-cli upload command for the sketch without a --port flag.
    For example:
    $ arduino-cli upload --fqbn arduino:avr:leonardo --verbose "/tmp/BareMinimum"
    
  3. Watch the command output until you see the following line:
    Waiting for upload port...
    
  4. Press and release the reset button on the board twice quickly to activate the bootloader.

The upload should now finish successfully. After that the board will produce a port once more and you can go back to uploading via the standard procedure.

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UPDATE: Fixed by 862bbe2

updated_upload_port may be different than upload target port when uploading to boards that produce multiple ports

Some Arduino boards produce multiple ports. The algorithm for identifying updated_upload_port selects the first of these to appear after an upload.

🐛 This port may be different from the one the user specified via the upload request, which will result in the updated_upload_port value being different than expected.

To reproduce

Equipment

  • Board that produces multiple ports, such as:
    • Nano ESP32
    • Teensy board (when running a sketch that was compiled with the usb custom board option set to one of the "serial" options)

Demo

$ ./arduino-cli version
arduino-cli.exe  Version: git-snapshot Commit: e9e5fbdd Date: 2023-08-16T03:41:25Z

$ ./arduino-cli sketch new /tmp/BareMinimum

[...]

$ ./arduino-cli board list
Port      Protocol Type              Board Name                FQBN                    Core
1-7.1.4.1 dfu      USB DFU           Arduino Nano ESP32        arduino:esp32:nano_nora arduino:esp32
COM11     serial   Serial Port (USB) Arduino Nano ESP32        arduino:esp32:nano_nora arduino:esp32

$ ./arduino-cli upload --fqbn arduino:esp32:nano_nora --protocol dfu --port 1-7.1.4.1 PERTILLISCH/sketches/BareMinimum/

dfu-util 0.11-arduino4

Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2021 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http://sourceforge.net/p/dfu-util/tickets/

Opening DFU capable USB device...
Device ID 2341:0070
Device DFU version 0101
Claiming USB DFU Interface...
Setting Alternate Interface #0 ...
Determining device status...
DFU state(2) = dfuIDLE, status(0) = No error condition is present
DFU mode device DFU version 0101
Device returned transfer size 4096
Copying data from PC to DFU device
Download        [=========================] 100%       286064 bytes
Download done.
DFU state(7) = dfuMANIFEST, status(0) = No error condition is present
DFU state(2) = dfuIDLE, status(0) = No error condition is present
Done!
New upload port: COM11 (serial)

$ ./arduino-cli board list
Port      Protocol Type              Board Name                FQBN                    Core
1-7.1.4.1 dfu      USB DFU           Arduino Nano ESP32        arduino:esp32:nano_nora arduino:esp32
COM11     serial   Serial Port (USB) Arduino Nano ESP32        arduino:esp32:nano_nora arduino:esp32

🐛 updated_upload_port was COM11 even though 1-7.1.4.1 was specified via the --port flag.

Expected behavior

updated_upload_port is the same (or equivalent) to the target port when such a port is available after upload.

Any additional ports produced by the board are only selected for updated_upload_port if there is no closer port match.

Arduino CLI version

e9e5fbd

Operating system

  • Windows
  • Linux
  • macOS

Operating system version

  • Windows 11
  • Ubuntu 22.04
  • macOS Ventura

Additional context

Admittedly the demonstration I provided of a 1-7.1.4.1 -> COM11 port change when uploading to Nano ESP32 is not very compelling. The user of this specific board would typically be better off with the serial protocol port selected when available. However, unexpected port selection changes in Arduino IDE when using Nano ESP32 was one of the faults that were specifically targeted for resolution by this work. I don't know how Arduino CLI behaves in this situation, but the direction of the port change fault as produced by the Arduino IDE port selection system was nondeterministic, meaning that some users experienced an unexpected change from the serial to the dfu protocol port, which caused Serial Monitor to no longer function (since there is no pluggable monitor for dfu protocol).

Even if the direction of the change is deterministically to the serial protocol port, for other boards this might be harmful. For example, the teensy protocol port is preferred over the serial port when using a Teensy board and Arduino CLI returns the serial port after uploading to the teensy port.

arduino/discovery/discovery.go Outdated Show resolved Hide resolved
commands/upload/upload.go Outdated Show resolved Hide resolved
commands/upload/upload.go Outdated Show resolved Hide resolved
commands/upload/upload.go Outdated Show resolved Hide resolved
internal/algorithms/channels.go Outdated Show resolved Hide resolved
@cmaglie
Copy link
Member Author

cmaglie commented Aug 16, 2023

Panic when upload causes disappearance of port
On boards with native USB, the USB stack that produces the USB CDC serial port used for uploading is running on the same microcontroller as the sketch program. This means that, when working with these board, it is common for sketch uploads to cause the disappearance of the port, either unexpectedly due to a bug such as a divide by zero or expectedly such as when putting the MCU to sleep.

Great catch, I've just pushed a commit to fix this!

The new algorithm takes into account the case where a single board may
expose multiple ports, in this case the selection will increase priority
to ports that:

  1. have the same HW id as the user specified port for upload
  2. have the same protocol as the user specified port for upload
  3. have the same address as the user specified port for upload
@cmaglie
Copy link
Member Author

cmaglie commented Aug 16, 2023

updated_upload_port may be different than upload target port when uploading to boards that produce multiple ports

updated_upload_port may be different than upload target port when uploading to boards that produce multiple ports
Some Arduino boards produce multiple ports. The algorithm for identifying updated_upload_port selects the first of these to appear after an upload.

🐛 This port may be different from the one the user specified via the upload request, which will result in the updated_upload_port value being different than expected.

Ok, I've pushed also a patch for this issue, the detection algorithm now prioritizes ports having (in order of importance):

  1. the same HW ID of the upload port
  2. the same protocol of the upload port
  3. the same address as the upload port

BTW to allow this selection I needed to introduce a delay to allow the discoveries to detect the set of available ports and report them to the watcher. The initial delay is 5 seconds and it may be cut to 1 sec if a port with matching HW ID is found. From my preliminary tests, it seems to work fine but I'm not 100% sure about this last point (reducing the timeout to 1 sec), so more testing is very welcome.

About testing, I'd like to have feedback also:

  • on port detection delays, since it may take up to 5 seconds AFTER the upload, I'd like to understand if this is noticeable
  • in case the port selection is not correct, please use the --log --log-level trace flag and paste the logs after the line:
TRAC[0005] Detecting new board port after upload         task=port_detection

For example here is the trace I'm interested in:

[...]
TRAC[0005] Detecting new board port after upload         task=port_detection
TRAC[0005] Ignored watcher event before upload           event="&{add /dev/ttyACM1 builtin:serial-discovery}" task=port_detection
TRAC[0005] Ignored watcher event before upload           event="&{add /dev/ttyS0 builtin:serial-discovery}" task=port_detection
DEBU[0005] LAST: map[/dev/ttyACM1:true /dev/ttyS0:true]  phase="board reset"
DEBU[0005] TOUCH: /dev/ttyACM1                           phase="board reset"
INFO[0005] Performing 1200-bps touch reset on serial port /dev/ttyACM1  phase="board reset"
TRAC[0006] Executing upload tool: "/home/megabug/.arduino15/packages/arduino/tools/bossac/1.9.1-arduino5/bossac"  --port=ttyACM1 -U -e -w "/tmp/arduino/sketches/002050EAA7EFB9A4FC451CDFBC0FA2D3/Blink.ino.bin" -R  phase=upload
Erase flash

Done in 0.001 seconds
Write 46744 bytes to flash (12 pages)
[==============================] 100% (12/12 pages)
Done in 2.966 seconds
TRAC[0009] Upload successful                            
INFO[0010] from discovery builtin:serial-discovery received message type: remove, porta: /dev/ttyACM1 
TRAC[0010] Candidate port is no longer available         event="&{remove /dev/ttyACM1 builtin:serial-discovery}" task=port_detection
TRAC[0010] User-specified port has been disconnected, now waiting for upload port, timeout extended by 5 seconds  task=port_detection
INFO[0010] from discovery builtin:serial-discovery received message type: add, porta: /dev/ttyACM0 
TRAC[0010] Found new upload port candidate (prio=100)    event="&{add /dev/ttyACM0 builtin:serial-discovery}" task=port_detection
TRAC[0010] New candidate port match the desired HW ID, timeout reduced to 1 second.  task=port_detection
TRAC[0011] Timeout waiting for candidate port            selected_port=/dev/ttyACM0 task=port_detection

@per1234 per1234 dismissed their stale review August 17, 2023 01:16

Previous reviews have been resolved. Thanks!

@cmaglie cmaglie merged commit 38479dc into arduino:master Aug 18, 2023
95 checks passed
@cmaglie cmaglie deleted the board_port_after_upload branch August 18, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
criticality: highest Of highest impact priority: high Resolution is a high priority topic: code Related to content of the project itself topic: gRPC Related to the gRPC interface type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Identify final board port address in machine readable upload command response
4 participants